pgconn: drain socket in asyncClose to avoid RST on context cancel#2585
Merged
Conversation
When a context is canceled while the server is still streaming rows, asyncClose() sends CancelRequest, writes Terminate, and closes the underlying net.Conn. At that point the kernel receive buffer typically still holds DataRow messages that were already in flight before the cancel landed. Closing a TCP socket with unread data causes the kernel to discard the receive queue and send RST instead of FIN (RFC 1122 4.2.2.13), which surfaces on the server or proxy as "connection reset by peer". Drain the socket with io.Copy(io.Discard, conn) after sending Terminate. The existing 15s deadline bounds the read; in the normal case the server stops producing rows after CancelRequest, processes Terminate, and closes its side, so the drain returns on io.EOF and the deferred Close() emits a clean FIN. Add TestAsyncCloseDrainsBeforeClose, which floods a mock client with DataRows, cancels mid-stream, and asserts the server observes Terminate followed by io.EOF rather than ECONNRESET. The test reproduces the reported "connection reset by peer" without the fix. Fixes jackc#2584 Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a context is canceled while the server is mid-stream,
asyncClose()sendsCancelRequest, writesTerminate, and closes the underlyingnet.Conn. At that point the kernel receive buffer typically still holdsDataRowmessages that were already in flight before the cancel landed. Closing a TCP socket with unread data in the receive queue causes the kernel to discard it and send RST instead of FIN (RFC 1122 §4.2.2.13). The peer (PostgreSQL, or a proxy such as pgdog or PgBouncer) sees this asconnection reset by peerand logs it as an error even though the disconnect was intentional.This change adds
io.Copy(io.Discard, pgConn.conn)after theTerminateflush inasyncClose(), draining whatever the server already sent. In the normal case the server stops producing rows shortly afterCancelRequest, processesTerminate, and closes its side; the drain returns onio.EOFand the deferredClose()emits a clean FIN. The existing 15-second deadline bounds the drain so a misbehaving server cannot block cleanup indefinitely.Behavior change
asyncClose()previously returned from its goroutine immediately after writingTerminate. It now blocks (in that background goroutine) until the server closes or the 15s deadline fires. Callers that wait onCleanupDone()(e.g. connection pools) may see slightly delayed slot reclamation against an unresponsive server. Against a healthy server the drain completes in one RTT plus whatever was already buffered.Testing
TestAsyncCloseDrainsBeforeClosespins up a mock backend that floods the client with ~16 MB ofDataRows, has the client cancel mid-stream, and asserts the server observesTerminatefollowed byio.EOF. Without the fix the test fails with the exact error from the issue report:With the fix it passes in ~100ms and is
-raceclean over repeated runs.AI disclosure
This change was drafted with assistance from Claude Code (Anthropic). I have reviewed the diff line by line, understand why closing with a non-empty receive buffer produces RST, why draining to
io.Discardresolves it, and why the existing deadline is the correct bound. I can answer questions about the change directly.Fixes #2584